feat: add MopDryerTrait for controlling the dock mop dryer - #952
Conversation
Adds a switch trait for starting and stopping a mop drying cycle via APP_SET_DRYER_STATUS, gated on the dock being able to dry. The dryer has no dedicated query command: whether a cycle is running is reported as dry_status on the device status. The trait therefore holds the status trait, reads is_on from it, and refreshes through it, applying the commanded value optimistically like the other switch traits. Named MopDryerTrait rather than DryerTrait to stay clear of the Zeo washer/dryer appliance support, which has its own dryer concepts.
There was a problem hiding this comment.
Thank you for putting this together @dschuld! Exposing mop drying controls via a trait is a very welcome addition and resolves a longstanding request (#905, #262). (Note: I am currently experimenting with agentic code review to help review PRs, so please let me know if anything in the feedback looks off).
I have left a few inline comments on the architecture, trait contract, and state encapsulation.
| return dock_features.is_dryable | ||
|
|
||
|
|
||
| class MopDryerTrait(common.V1TraitMixin, common.RoborockSwitchBase): |
There was a problem hiding this comment.
Subclassing V1TraitMixin without declaring the class variables command: ClassVar[RoborockCommand] and converter: V1TraitDataConverter (and without MopDryerTrait subclassing a RoborockBase dataclass) leaves the mixin contract incomplete. V1TraitMixin.refresh() expects to run merge_trait_values(self, ...) on dataclass fields.
As you noted in the PR description, the V1 protocol actually supports APP_GET_DRYER_SETTING and APP_SET_DRYER_SETTING (documented in docs/V1_API_COMMANDS.md#app_get_dryer_setting).
Two possible paths here:
- Option A (Preferred / Complete Trait): Model a self-contained
MopDryerTraitbacked byAPP_GET_DRYER_SETTINGwith aDryerSetting(RoborockBase)dataclass. This exposes both the dryer switch (enable/disable) and duration settings while completely avoiding coupling withStatusTrait. - Option B (Lightweight Switch): If keeping this strictly to a start/stop cycle switch without settings, avoid inheriting from
V1TraitMixinsince its query/converter contract isn't used. Instead, accept the RPC channel explicitly.
There was a problem hiding this comment.
Thanks for the detailed feedback and directions! I went with option A and implemented a proper trait. I captured APP_GET_DRYER_SETTING from my dock. There are now MopDryerSetting and MopDryerProfile dataclasses in v1_containers.py, and the trait declares command and converter and inherits the generic refresh().
| """Start drying the mop.""" | ||
| await self.rpc_channel.send_command(RoborockCommand.APP_SET_DRYER_STATUS, params={_STATUS_PARAM: 1}) | ||
| # Optimistic update to avoid an extra refresh | ||
| self._status_trait.dry_status = 1 |
There was a problem hiding this comment.
Mutating _status_trait.dry_status directly across trait boundaries leaks encapsulation and bypasses any listeners/subscribers registered on StatusTrait (like TraitUpdateListener).
Per repository guidelines, traits should manage their own internal state (e.g., maintaining an internal _is_on attribute on MopDryerTrait initialized or refreshed as needed) rather than mutating an external trait's cached telemetry.
There was a problem hiding this comment.
Fixed. The trait is now a MopDryerSetting dataclass and updates its own self.status, the same way ChildLockTrait does with lock_status. It no longer holds a reference to StatusTrait at all, so nothing bypasses the update listeners.
| ) | ||
| # The command result is applied optimistically to avoid an extra refresh | ||
| assert device.v1_properties.status.dry_status == expected_status | ||
| assert mop_dryer.is_on is bool(expected_status) |
There was a problem hiding this comment.
Tests should verify the public contract and behavior of the trait under test (mop_dryer.is_on, emitted RPC commands) rather than asserting on cross-trait side-effects in device.v1_properties.status.dry_status.
There was a problem hiding this comment.
Fixed. The tests now assert only on commands emitted and the trait's own state, no references to device.v1_properties.status. The refresh test uses the APP_GET_DRYER_SETTING payload captured from my dock and checks the nested on/off profiles parse correctly.
|
Marking draft, please re-open for review when you're ready for me to take another look. |
… status
Model the trait on APP_GET_DRYER_SETTING with a MopDryerSetting dataclass
so it fulfils the V1TraitMixin contract (command, converter, dataclass
fields) rather than borrowing state from StatusTrait.
The switch now controls the auto mop-drying setting: is_on reflects
`status`, and enable/disable send APP_SET_DRYER_SETTING with a partial
`{"status": N}`, which the device accepts and merges without touching
the `on`/`off` profiles. Starting and stopping a drying cycle directly
moves to start_dry/stop_dry via APP_SET_DRYER_STATUS. Whether a cycle is
running remains reported by `dry_status` on the device status.
The trait no longer takes constructor arguments, so it is created by the
generic discovery loop and the special-case block in discover_features
is removed.
Tests assert only on the commands sent and the trait's own state; the
refresh test uses a payload captured from a real dock.
Summary
A switch trait for starting and stopping a mop drying cycle on docks that support
drying, via
APP_SET_DRYER_STATUS.dry_statusis already readable. It was annotated with dps metadata in #740 and isconsumed by Home Assistant through
is_field_supported(StatusV2, StatusField.DRY_STATUS). But there is no trait behind the write side, so callers have to fall back to a raw command.The trait is created in the special-case block in
discover_features()alongsideWashTowelModeTraitandObstaclePhotoTrait, since it takes a constructor argument.It is gated on
dock_features.is_dryable.Background
#262 asked for this in Dec 2024 and was closed by the reporter with:
APP_SET_DRYER_SETTINGconfigures the dry duration;APP_SET_DRYER_STATUSstarts andstops the cycle. That finding never made it into the library. I've confirmed the
command works against my own dock.
Open Issues / Design Choices
1. Where the state comes from. Unlike the other switch traits, the dryer has no query
command of its own. If a cycle is running is reported as
dry_statusonStatusV2.So this trait holds the status trait, reads
is_onfrom it, and overridesrefresh()todelegate, leaving
V1TraitMixin'scommand/converterunused.That is a deliberate deviation from the pattern documented in the module docstring
("subclass
V1TraitMixinand aRoborockBasedataclass… you must define acommandclass variable"). I would like to point this out here rather than have it found in review.
The alternative is a self-contained trait backed by
APP_GET_DRYER_SETTINGwith a newDryerSettingdataclass, which would also expose the dry-duration setting. I didn't gothat way because I have no sample of that response and didn't want to guess at the wire
format. I am happy to capture it from my dock and rework this if this approach is preferred.
Testing
50 new tests in
tests/devices/traits/v1/test_mop_dryer.py:is_dryableis true, and absent for all whereit is false (parametrized over the full
RoborockDockTypeCodeenum)is_onmapping fordry_statusofNone/0/1enable()/disable()sendAPP_SET_DRYER_STATUSwith the right params and apply theoptimistic update
refresh()delegates to the status trait100% line and branch coverage on the new module.
AI assistance disclosure
This contribution was prepared with Claude Code assistance. I reviewed the submitted changes and test results and take responsibility for the contribution.
Closes #905